Fix #483, #484 and above all handle errors happening in search stream leaves. #487

fmassot · 2021-08-29T21:11:47Z

Bonus:

the leaf stream return a search error and not a tonic status
add tests at the root and leaf levels
add test on the cli (csv output)

Not included:

tests on collector: some important changes are coming soon and I would prefer to implement tests at this moment
tests on performance would be nice

quickwit-search/src/search_stream/root.rs

guilload

After reading this PR, I vote for changing SearchError::InternalError(String) to SearchError::InternalError(anyhow::Error).

quickwit-cli/tests/cli.rs

guilload · 2021-08-31T21:31:14Z

quickwit-cli/tests/cli.rs

+    assert_eq!(metadata_file_exist, false);
+
+    Ok(())
+}


We could also test that the split file no longer exists.

I think SearchError gets serialized/deserialized... anyhow cannot be serialized, hence the msg.

I like anyhow because it tends to do a better job at tracking the underlying error while we do a poor job at it. We could serialize the anyhow error as a string using the debug format, which displays the cause and a backtrace if one is present.

guilload · 2021-08-31T22:49:53Z

quickwit-search/src/client.rs

-                        result_sender.send(Ok(result)).map_err(|_| {
-                            SearchError::InternalError("Could not send leaf result".into())
-                        })?;
+                    loop {


I find this logic hard to follow. results_stream is a Streaming<T> struct which implements Stream. We can greatly simplify the whole thing:

let mut results_stream = grpc_client_clone .leaf_search_stream(tonic_request) .await .map_err(|tonic_error| parse_grpc_error(&tonic_error))? .into_inner() .take_while(|grpc_result| futures::future::ready(grpc_result.is_ok())) .map_err(|tonic_error| parse_grpc_error(&tonic_error)); while let Some(search_result) = results_stream.next().await { result_sender.send(search_result).map_err(|_| { SearchError::InternalError( "Sender closed, could not send leaf result.".into(), ) })?; }

I think this is actually going to swallow the last error :x

This is what I want rust-lang/rust#62208. Anyway t_t

the last element of the stream is None so I think it should work, I really like your version, I did not think about the take_while

guilload · 2021-08-31T22:52:36Z

quickwit-search/src/search_stream/collector.rs

@@ -168,11 +168,13 @@ impl FastFieldCollectorBuilder {
    }

    pub fn fast_field_to_warm(&self) -> Vec<String> {


Had this return type be a Set, we wouldn't have to do this. Is tag_fields on IndexConfig also a Vec instead of a Set?

guilload · 2021-08-31T23:05:16Z

quickwit-search/src/search_stream/leaf.rs

-            Result::<(), SearchError>::Ok(())
-        });
-    }
+    let request_clone = request.clone();


We could "own" that request and not clone it as you did in some other parts of this PR.

+1... Generally speaking if ownership is required, it is better to reflect it in the prototype. It does not necessarily avoid the clone, but at least it might.

guilload · 2021-08-31T23:06:12Z

quickwit-search/src/search_stream/leaf.rs

+        let mut stream =
+            leaf_search_results_stream(request_clone, storage, splits, index_config).await;
+        while let Some(item) = stream.next().await {
+            if let Err(error) = result_sender.send(item) {


Just my personal preference, but I like the map_err(...)? pattern better.

I prefer the if let Err here.

The body here is just doing logging (so a side effect).
map_err is more about converting an error isn't it?

guilload · 2021-08-31T23:06:58Z

quickwit-search/src/search_stream/leaf.rs

+                storage.clone(),
+            )
+        })
+        .map(|(split, index_config_clone, request_clone, storage)| {


Why the double map? Readability?

readability and compiler issue I had but it's no more relevant.

guilload · 2021-08-31T23:14:14Z

I mostly made syntactical comments. The functional changes are great and good job on those tests.

Co-authored-by: Adrien Guillo <adrien@quickwit.io>

fmassot

After reading this PR, I vote for changing SearchError::InternalError(String) to SearchError::InternalError(anyhow::Error).

We serialize SearchError to pass it through the network so I'm not sure how to handle it on the serialization side. We could serialize the error as a string but then we're back to square one. Nevertheless, we could at least have real errors on the root because these errors are not serialized.

Right now, I have the feeling that we already need to improve the errors we are handling (SearchError, NodeSearchError), I found it a little bit messy.

fmassot · 2021-09-01T00:03:36Z

quickwit-search/src/search_stream/collector.rs

@@ -168,11 +168,13 @@ impl FastFieldCollectorBuilder {
    }

    pub fn fast_field_to_warm(&self) -> Vec<String> {


fulmicoton · 2021-09-01T08:01:40Z

Glad to see unit tests on thjis part of the code!

fmassot requested a review from evanxg852000 August 29, 2021 21:11

fmassot changed the title ~~Fix #483, #484 and above all handle errors happening in leaves.~~ Fix #483, #484 and above all handle errors happening in search stream leaves. Aug 29, 2021

fmassot force-pushed the fix-stream-search branch from db82dac to 9c55514 Compare August 30, 2021 14:34

evanxg852000 reviewed Aug 30, 2021

View reviewed changes

quickwit-search/src/search_stream/root.rs Outdated Show resolved Hide resolved

evanxg852000 approved these changes Aug 30, 2021

View reviewed changes

fmassot force-pushed the fix-stream-search branch from d519bd1 to f4c9d87 Compare August 30, 2021 20:24

fmassot mentioned this pull request Aug 30, 2021

Use futures::stream in leaf functions #452

Closed

fmassot force-pushed the fix-stream-search branch from f4c9d87 to c6287a7 Compare August 31, 2021 19:33

fmassot added 5 commits August 31, 2021 21:34

Fix #483, #484 and above all handle errors happening in leaves.

1b11708

Add test on search stream with a leaf error.

b05db93

Add test on leaf search stream result.

2447554

Add cli serve tests on a local file index.

d87b4ce

Fix typo in tests.

0b8b095

fmassot force-pushed the fix-stream-search branch from c6287a7 to 0b8b095 Compare August 31, 2021 19:37

guilload approved these changes Aug 31, 2021

View reviewed changes

Fix typo in test variable name.

17d1812

Co-authored-by: Adrien Guillo <adrien@quickwit.io>

fmassot commented Sep 1, 2021

View reviewed changes

fulmicoton approved these changes Sep 1, 2021

View reviewed changes

fulmicoton and others added 4 commits September 1, 2021 17:01

Merge branch 'main' into fix-stream-search

0b971e3

Improve code syntax/readability.

c51adaa

Merge branch 'main' into fix-stream-search

79d786e

Fix fmt

c1217c7

fmassot merged commit b76a468 into main Sep 1, 2021

fmassot deleted the fix-stream-search branch September 1, 2021 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #483, #484 and above all handle errors happening in search stream leaves. #487

Fix #483, #484 and above all handle errors happening in search stream leaves. #487

fmassot commented Aug 29, 2021 •

edited

Loading

guilload left a comment

guilload Aug 31, 2021

fulmicoton Sep 1, 2021

guilload Sep 1, 2021

guilload Aug 31, 2021

guilload Aug 31, 2021

guilload Aug 31, 2021

fmassot Sep 1, 2021 •

edited

Loading

guilload Aug 31, 2021

fmassot Sep 1, 2021

guilload Aug 31, 2021

fulmicoton Sep 1, 2021

guilload Aug 31, 2021

fulmicoton Sep 1, 2021

guilload Aug 31, 2021

fmassot Sep 1, 2021

guilload commented Aug 31, 2021

fmassot left a comment •

edited

Loading

fmassot Sep 1, 2021

fulmicoton commented Sep 1, 2021

		@@ -168,11 +168,13 @@ impl FastFieldCollectorBuilder {
		}

		pub fn fast_field_to_warm(&self) -> Vec<String> {

Fix #483, #484 and above all handle errors happening in search stream leaves. #487

Fix #483, #484 and above all handle errors happening in search stream leaves. #487

Conversation

fmassot commented Aug 29, 2021 • edited Loading

guilload left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassot Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilload commented Aug 31, 2021

fmassot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fulmicoton commented Sep 1, 2021

fmassot commented Aug 29, 2021 •

edited

Loading

fmassot Sep 1, 2021 •

edited

Loading

fmassot left a comment •

edited

Loading